NE-2334: Implement enhancement in OpenShift API to support for TLS curves in TLSProfile#2583
NE-2334: Implement enhancement in OpenShift API to support for TLS curves in TLSProfile#2583davidesalerno wants to merge 5 commits intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment |
|
Hello @davidesalerno! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/L
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
8c07857 to
16a5ecc
Compare
|
/cc @sanchezl |
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
16a5ecc to
6d3bb10
Compare
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
As discussed in slack, let's create a featuregate and enhancement to attach to this.
...zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs-CustomNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Add Default Curves to TLSProfiles
The Curves field has been added to the API schema, but all entries in the TLSProfiles map should be updated to include default curves. Currently, only the Ciphers field is populated in each profile.
Recommended Default Curves
All three TLS profiles (TLSProfileOldType, TLSProfileIntermediateType, TLSProfileModernType) should include these curves by default:
Curves: []string{
"X25519",
"P-256",
"P-384",
"X25519MLKEM768",
},Go TLS Constant to Configuration String Mapping
The PR should document the mapping between Go's crypto/tls constants and the configuration strings used in the Curves field:
| Go Constant | Configuration String | Description |
|---|---|---|
tls.X25519 |
X25519 |
Curve25519 (recommended) |
tls.CurveP256 |
P-256 |
NIST P-256 (secp256r1) |
tls.CurveP384 |
P-384 |
NIST P-384 (secp384r1) |
tls.X25519MLKEM768 |
X25519MLKEM768 |
Post-Quantum Cryptography hybrid |
Given that the cipher names expected in this configuration are based on OpenSSL names, I suggest OpenSSL versions of the strings. OpenSSL has a few alias defined for some of the curves. I've picked the aliases that match the NIST names when I thought it was appropriate.
Documentation Update Needed
The field documentation should include an example showing how to configure a PQC-only (Post-Quantum Cryptography) TLS profile. Users wanting to enforce PQC-only encryption would create a custom TLS profile:
# Example: Force PQC-only encryption
apiVersion: config.openshift.io/v1
kind: APIServer
spec:
tlsSecurityProfile:
type: Custom
custom:
ciphers:
- TLS_AES_128_GCM_SHA256
- TLS_AES_256_GCM_SHA384
- TLS_CHACHA20_POLY1305_SHA256
curves:
- X25519MLKEM768 # PQC-only: only hybrid quantum-resistant curve
minTLSVersion: VersionTLS13This configuration ensures that only connections using the post-quantum hybrid key exchange can be established.
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
6d3bb10 to
9754d52
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new exported type 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
config/v1/types_tlssecurityprofile.go (1)
216-225: Clarify default behavior whenCurvesis unset
Curvesis marked+optional, but the godoc only explains usage and not what happens when the field is omitted (or effectively empty). That makes it unclear whether implementations fall back to library/platform defaults, profile‑specific defaults, or treat it as misconfiguration.Consider extending the comment to explicitly document the default, e.g. that omitting
curvesmeans “use the platform’s/default profile’s curve set (subject to change between releases)”.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml (1)
147-159: Schema added; verify MCO/kubelet honor it end‑to‑end.This addresses the earlier concern about KubeletConfig coverage. Please confirm Machine Config Operator plumbs TLSProfileSpec.Curves through to kubelet (or ignores safely) and document any TLS 1.3 caveats for kubelet.
🧹 Nitpick comments (2)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
333-345: Consistent addition; mirror example/documentation.Change is consistent. Consider adding a brief note on naming (IANA/openssl-style like X25519, P-256) and reference that operands may ignore/trim unsupported entries; also add curves to the example snippet above for parity.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
264-276: Default profile CRD aligned; minor doc tweak suggested.Looks good. As with other CRDs, consider adding curves to the example block and a brief note on accepted names to avoid user confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (20)
config/v1/types_tlssecurityprofile.go(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(4 hunks)operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml(2 hunks)operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamloperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yamlconfig/v1/types_tlssecurityprofile.goconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yamloperator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yamlopenapi/openapi.jsonconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlopenapi/generated_openapi/zz_generated.openapi.goconfig/v1/zz_generated.swagger_doc_generated.goconfig/v1/zz_generated.deepcopy.go
🔇 Additional comments (19)
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml (1)
264-276: TLS curves field mirrors existing ciphers semantics correctly
curvesis modeled as an atomic[]stringwith clear docs and consistent placement undertlsSecurityProfile.custom; this aligns with howciphersis exposed and looks good from an API and compatibility standpoint.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
333-345: Consistent exposure ofcurvesin CustomNoUpgrade APIServer CRDThe
curvesarray is defined identically to the feature-gated CRD ([]stringwithx-kubernetes-list-type: atomicand matching description), keeping profiles consistent across variants.config/v1/zz_generated.swagger_doc_generated.go (1)
2975-2982: Swagger doc forcurvesmatches schema and intentThe new
curvesentry inTLSProfileSpec’s Swagger doc accurately describes usage and mirrors the CRD text, keeping the public API documentation consistent with the new field.operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml (1)
1980-1992: Curves schema for IngressController looks consistent and well‑shapedThe new
curvesarrays under.spec.tlsSecurityProfile.customand.status.tlsProfilemirror the existingciphersfield (description, list semantics, and placement) and align with the GoTLSProfileSpec. No issues from a schema or maintainability perspective.Also applies to: 3252-3264
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
333-345: APIServer TechPreviewcurvesfield matches TLS profile contractThe
curvesfield is added alongsidecipherswith matching description and list semantics, which keeps the CRD schema aligned withTLSProfileSpecand other manifests. Looks good.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
333-345: DevPreview APIServer CRDcurvesaddition is consistentThe
curvesproperty undertlsSecurityProfile.customis structurally identical to the existingciphersfield and to the TechPreview/other CRDs. This keeps the API surface uniform across feature sets.machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml (1)
148-160: KubeletConfig TLScurvesfield is correctly wired into the CRDThe new
curvesarray undertlsSecurityProfile.custommatches the existingciphersfield’s structure and the sharedTLSProfileSpeccontract, so the feature is exposed consistently to KubeletConfig consumers.config/v1/zz_generated.deepcopy.go (1)
6222-6233: TLSProfileSpec Curves deepcopy mirrors Ciphers pattern correctlyDeep copy of
Curvesuses the standardmake+copyguarded by a nil check, matchingCiphersand avoiding slice aliasing. Looks correct for the new field.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml (1)
135-159: KubeletConfig TLS custom.curves field is well-formed and consistentThe new
tlsSecurityProfile.custom.curvesarray mirrors the existingciphersschema (type, list-type, and style of description), so the CRD remains consistent and backward compatible.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
321-345: APIServer (CustomNoUpgrade) TLS curves schema matches existing patternsThe
custom.curvesfield is defined analogously tocustom.ciphers(string array, atomic list, clear description), so the extension of the TLS profile is coherent and safe.payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
252-276: APIServer (Default) curves field keeps TLS profile schemas alignedThe new
tlsSecurityProfile.custom.curvesfield is structurally identical to the one in other APIServer CRDs, maintaining consistency across feature sets without affecting existing behavior.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
321-345: DevPreview APIServer CRD curves field is consistent with other variantsThe
custom.curvesarray is defined consistently withciphersand with the corresponding fields in the Default/CustomNoUpgrade CRDs, so schema behavior stays uniform across feature sets.openapi/generated_openapi/zz_generated.openapi.go (2)
11831-11850: Curves field schema looks correct and consistent with existing TLS profile patternsArray-of-string with
x-kubernetes-list-type: "atomic"and a clear description fits how similar list fields (e.g., ciphers) are modeled here. No issues from a schema or maintainability perspective.
20311-20330: Duplicate curves field for TLSProfileSpec is correctly mirroredThis definition mirrors the earlier curves schema, keeping TLSProfileSpec and CustomTLSProfile aligned. The structure and description are consistent and look good.
operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml (2)
3235-3247: Status parity: ensure controller fills status.tlsProfile.curves.Schema addition is correct. Please confirm the ingress operator sets this field so users can observe effective groups at runtime.
1974-1986: Curves field is correctly implemented repo-wide; documentation suggestions are optional.Verification confirms the Curves field has been properly propagated:
- Go struct definition includes
Curves []stringwithjson:"curves,omitempty"(line 225 in types_tlssecurityprofile.go)- Generated deepcopy code includes Curves field handling
- CRD manifests include curves schema in both
spec.tlsSecurityProfile.customandstatus.tlsProfilesections with matching structure to ciphers (type array, x-kubernetes-list-type: atomic)- No CRDs have ciphers without curves—consistent all-or-nothing propagation
The original suggestions to clarify naming conventions (X25519, P-256, etc.) and extend example snippets are documentation improvements, not required code changes.
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
333-345: LGTM; feature-gated schema stays aligned with Default/TPN variants.No issues. Keep the curves description aligned across CRDs to avoid drift.
Please run the repo-wide script from the ingresscontrollers comment to confirm no CRD missed curves.
openapi/openapi.json (2)
6019-6027: Curves field additions look correct, but clarify item default semantics.The
curvesfield is properly defined as an array of strings with appropriate metadata (x-kubernetes-list-type: atomic) and helpful example documentation. The field appears in two locations (hunks 1 & 2), which aligns with the PR's goal of adding curve support to multiple TLSProfile contexts.However, the
"default": ""for items (lines 6024, 11001) is semantically unclear—an empty string is not a valid elliptic curve identifier. Verify whether this is a required OpenAPI convention for array items or if it should be omitted. If retained, consider updating the description to clarify that operators should never rely on this default.Also applies to: 10996-11004
14289-14358: Clarify scope of FormatMarkerExamples schema addition.The new
FormatMarkerExamplesschema (hunk 3) and its reference (hunk 4) introduce comprehensive documentation for Kubebuilder format markers (14 format marker examples: base64, CIDR, date/time, email, hostname, IP addresses, MAC, password, URI, UUID variants). While the enriched summary indicates this "aligns with broader TLS/curves-related schema enhancements," the schema itself is disconnected from the curves functionality described in the PR objectives.Questions:
- Is FormatMarkerExamples part of the core curves feature, or is this a separate documentation addition that should be scoped in its own PR?
- Is this schema auto-generated or manually maintained? If auto-generated, confirm the generation tool was run.
- The CVE references (CVE-2021-29923, CVE-2024-24790) in the descriptions are helpful; confirm they are kept up-to-date across future releases.
Also applies to: 14448-14451
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
9754d52 to
58da6c8
Compare
openshift-ci
bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml (1)
148-178: CRD enum forcurvesdoes not allow documentedX25519MLKEM768value.The
curvesdescription and example mentionX25519MLKEM768, but theitems.enumonly listsX25519,X448,P-256,P-384, andP-521. A KubeletConfig using the documented PQC example will therefore fail schema validation.Once you add
X25519MLKEM768to theTLSCurvekubebuilderEnuminconfig/v1/types_tlssecurityprofile.go, please regenerate this CRD so thecurves.items.enumincludesX25519MLKEM768as well.operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml (1)
3270-3300: Duplicate of enum/docs mismatch in status.tlsProfile.curves.Apply the same enum addition here to keep status schema consistent with spec and docs.
enum: - X25519 - X448 - P-256 - P-384 - P-521 + - X25519MLKEM768payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
333-363: Enum mismatch: X25519MLKEM768 documented but not enumerated (same as Default variant).This file shows the identical documentation-enum mismatch as the Default CRD variant: the description references X25519MLKEM768 as a default curve, but the enum does not permit it.
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
333-363: Enum mismatch: X25519MLKEM768 documented but not enumerated (consistent across all variants).Like the Default and CustomNoUpgrade variants, this featuregated CRD also documents X25519MLKEM768 as a default curve while omitting it from the enum validation list. This systematic issue across all CRD manifests suggests a source-level problem that needs correction.
🧹 Nitpick comments (1)
payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml (1)
111-116: Avoid referencing Modern defaults where Modern isn’t supported here.This CRD states only Old and Intermediate are supported, but the curves description discusses Modern defaults. Consider trimming that sentence to reduce confusion.
Also applies to: 147-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (20)
config/v1/types_tlssecurityprofile.go(5 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(4 hunks)operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml(2 hunks)operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- openapi/generated_openapi/zz_generated.openapi.go
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
- machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml
- config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml
- operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
- config/v1/zz_generated.swagger_doc_generated.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yamloperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlconfig/v1/zz_generated.deepcopy.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlconfig/v1/types_tlssecurityprofile.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml
🧬 Code graph analysis (1)
config/v1/zz_generated.deepcopy.go (1)
config/v1/types_tlssecurityprofile.go (1)
TLSCurve(211-211)
🔇 Additional comments (5)
config/v1/types_tlssecurityprofile.go (1)
239-268: Curves field docs and defaulting behavior look good.The
Curvescomment now clearly describes defaults for Old/Intermediate/Modern, states that Custom has no defaults, and explains behavior when the optional field is unset. This matches howTLSProfilesis initialized and addresses the prior request to document default behavior for an optional field.config/v1/zz_generated.deepcopy.go (1)
6222-6233: Deep-copy logic forCurvesis correct and consistent.The new deepcopy block for
Curvesmirrors the existingCiphershandling (allocate new slice, thencopy), soTLSProfileSpeccopies will not share slice storage.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
333-363: Based on my web search results, I have found critical information that makes the original review comment incorrect.The OpenShift APIServer CR specification does NOT include a user-facing "curves" field. The tlsSecurityProfile Custom profile only exposes
ciphersandminTLSVersion—curve preferences cannot be set by users via the APIServer CR.This means:
- The CRD file likely contains aspirational or incorrect documentation/examples
- Adding X25519MLKEM768 to an enum for a non-existent field won't solve any real problem
- The actual issue is that the documentation shows a curves configuration example that cannot be used
However, the file is auto-generated (indicated by the "zz_generated" prefix), so these issues likely stem from incorrect source specifications used to generate the CRD.
The APIServer CR tlsSecurityProfile Custom profile does not expose curve preferences. The documentation example showing curves configuration is incorrect—users cannot configure curves via the APIServer CR at all. This is a documentation/generation issue rather than a missing enum value.
The review comment's premise is flawed. Rather than adding X25519MLKEM768 to an enum for a non-existent field, the root cause—incorrect documentation showing unsupported configuration—should be addressed in the source specification that generates this CRD file.
Likely an incorrect or invalid review comment.
openapi/openapi.json (2)
6019-6027: Curves fields addition aligns with PR objectives.The "curves" field additions to CustomTLSProfile and TLSProfileSpec correctly mirror the structure of the existing "ciphers" field, with appropriate array type and
x-kubernetes-list-type: atomicannotation. The description is comprehensive, including default curves for each profile type and a practical YAML example with post-quantum cryptography curves.Also applies to: 10996-11004
14289-14358: Unexpected FormatMarkerExamples addition unrelated to PR objectives.Hunks 3 & 4 introduce a
FormatMarkerExamplesschema definition and field that are unrelated to the TLS curves feature described in the PR objectives. This appears to be an unintended inclusion in the PR.Please clarify:
- Are the FormatMarkerExamples changes intentional or accidental?
- Is this file auto-generated (e.g., via Swagger generation)? If so, these hunks should likely be removed and the file regenerated from source definitions rather than manually edited.
Also applies to: 14448-14451
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml
Outdated
Show resolved
Hide resolved
...z_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml
Outdated
Show resolved
Hide resolved
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
58da6c8 to
eba1e60
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml (1)
148-179: Consider clarifying curves description vs Kubelet profile support noteThe
curvesdescription generically states that TLSProfiles Old, Intermediate, and Modern include default curves, while the surroundingtlsSecurityProfiledocs forKubeletConfigsay only Old and Intermediate profiles are currently supported. If Modern truly isn’t honored by kubelet, consider tweaking this description (or adding a brief note) to avoid implying Modern is fully supported here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (20)
config/v1/types_tlssecurityprofile.go(5 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)machineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(7 hunks)operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml(2 hunks)operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
- openapi/generated_openapi/zz_generated.openapi.go
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
- config/v1/zz_generated.swagger_doc_generated.go
- payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
- machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yaml
- operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/types_tlssecurityprofile.gooperator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlconfig/v1/zz_generated.deepcopy.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests/kubeletconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlpayload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yamlopenapi/openapi.jsonconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml
🧬 Code graph analysis (1)
config/v1/zz_generated.deepcopy.go (1)
config/v1/types_tlssecurityprofile.go (1)
TLSCurve(211-211)
🔇 Additional comments (13)
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
333-364: curves schema for KMSEncryptionProvider looks correct and consistentThe
curvesfield mirrorsciphersstructurally, uses theTLSCurveenum with the expected values, and applies appropriatemaxItemsandx-kubernetes-list-type: atomic; this is a sound extension of the TLS profile schema.config/v1/zz_generated.deepcopy.go (1)
6201-6212: Deep-copy logic for Curves slice is correct
TLSProfileSpec.DeepCopyIntonow allocates a new[]TLSCurveandcopys elements whenCurvesis non-nil, matching the existing pattern forCiphersand providing correct value semantics for the new field.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
333-364: APIServer DevPreview CRD curves field is well-formed and aligned with APIThe added
curvesarray undertlsSecurityProfile.customcorrectly exposes theTLSCurveenum, matches the other APIServer CRDs, and provides a detailed description and example without altering existing behavior.operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml (1)
2022-2053: Curves spec/status schemas are consistent and aligned with TLSCurve.Both
spec.tlsSecurityProfile.custom.curvesandstatus.tlsProfile.curvesshare the same enum set,maxItems: 5, andx-kubernetes-list-type: atomic, which keeps CRD validation and status reporting in sync.Also applies to: 3351-3381
config/v1/types_tlssecurityprofile.go (2)
205-224: TLSCurve enum and Curves field cleanly extend the TLS profile API.Enum values match the documented curves, and the optional
Curves []TLSCurvewithMaxItems=5andlistType=atomicintegrates without impacting existing TLSProfileSpec consumers.Also applies to: 237-267
338-343: Default TLSProfiles correctly seed curves to the documented set.Old, Intermediate, and Modern profiles all initialize
Curvesto{TLSCurveX25519, TLSCurveP256, TLSCurveP384, TLSCurveX25519MLKEM768}, which matches the comments and respects the MaxItems constraint.Also applies to: 360-365, 374-379
payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
264-295: APIServer Default CRD curves field matches the TLSCurve API contract.The enum values,
maxItems: 5, andx-kubernetes-list-type: atomicalign with the GoTLSCurvedefinition and other manifests, so CRD validation will accept all supported curves consistently.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
333-364: CustomNoUpgrade APIServer CRD keeps curves schema in sync with TLSCurve.The
curvesfield’s enum,maxItems, and list-type mirror the core TLSCurve definition, ensuring this feature-set variant validates the same curve set as the default CRD.payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs.crd.yaml (1)
147-178: KubeletConfig tlsSecurityProfile.custom.curves schema is consistent with shared TLSCurve semantics.The enum values, list constraints, and description align with the common TLSProfileSpec behavior, so kubelet configs can configure the same validated curve set as other components.
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/AAA_ungated.yaml (1)
264-295: Curves field properly integrated with complete enum.The curves field addition is well-structured: all five supported curves are correctly enumerated (including X25519MLKEM768), the documentation clearly explains defaults and provides a PQC example, and the constraint (maxItems: 5) aligns with the available options. The field integrates properly alongside the existing ciphers and minTLSVersion configuration. Earlier concerns about X25519MLKEM768 being undocumented have been resolved by its inclusion in the enum and comprehensive documentation.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml (1)
264-295: Consistent curves implementation across CRD variants.This CRD manifest properly mirrors the feature-gated variant, maintaining identical curves field definition, enum values, and documentation. The consistency between feature-gated and default CRD manifests ensures uniform API behavior across cluster configurations.
openapi/openapi.json (2)
25382-25382: Unable to verify PR scope due to repository access limitations.I attempted to verify whether the OSImageStream changes are unrelated to the stated TLS curves support objective, but encountered persistent repository access failures that prevent me from:
- Accessing the PR description and title to confirm the stated objective
- Examining the git diff to identify hunks 4-7 and their actual content
- Verifying the relationship between OSImageStream and TLS curves changes
Without access to PR metadata and the codebase, I cannot definitively confirm or refute the claim that these schema additions are out of scope. To complete this verification, you or the repository maintainers should:
- Review the PR description to confirm whether OSImageStream changes are listed as part of the intended scope
- If the PR objective is solely "Add TLS curves support," confirm whether OSImageStream is a prerequisite or truly a separate concern
- Check the git history to understand when these schema definitions were intended to be added
6027-6035: The review comment's core suggestion is sound, but the proposed enum values require verification against the actual Go source code.Based on web research:
P-521 is problematic: Red Hat/OpenShift documentation lists supported TLS 1.3 curves as X25519, P-256, and P-384. P-521 does not appear in their documentation, suggesting it may not be a supported curve in OpenShift's TLS configuration.
Naming convention mismatch: The proposed enum uses curve aliases (X25519, P-256, P-384) rather than RFC/IANA standardized names (x25519, secp256r1, secp384r1). OpenAPI schemas typically use standardized names for interoperability, though this may be intentional for this API.
X25519MLKEM768 caveat: This hybrid quantum-resistant curve is conditionally supported based on Go version (Go 1.24+), which may make it unsuitable for a fixed enum or require version-gating.
The review comment cannot be fully verified without access to the Go source code defining the TLSCurve type. Manual verification is needed to confirm:
- The exact list of supported curves in the Go source
- The naming convention used in the actual type definition
- Whether all proposed enum values are accurate and complete
|
https://github.com/openshift/api/compare/58da6c8c1090bcc312e8f1bd3d8bca87cd47227d..eba1e60731dc58f0c314e2ef10ed38dd3f528c00 dropped X448. Was that intentional, perhaps because Go's crypto/tls package doesn't include support? Do we have agreement from stakeholders that the current set of curves is sufficient? api/config/v1/types_tlssecurityprofile.go Lines 205 to 224 in eba1e60 |
| // # Example: Force PQC-only encryption | ||
| // apiVersion: config.openshift.io/v1 | ||
| // kind: APIServer | ||
| // spec: | ||
| // tlsSecurityProfile: | ||
| // type: Custom | ||
| // custom: | ||
| // ciphers: | ||
| // - TLS_AES_128_GCM_SHA256 | ||
| // - TLS_AES_256_GCM_SHA384 | ||
| // - TLS_CHACHA20_POLY1305_SHA256 | ||
| // curves: | ||
| // - X25519MLKEM768 # PQC-only: only hybrid quantum-resistant curve | ||
| // minTLSVersion: VersionTLS13 |
There was a problem hiding this comment.
Does this render all right in the oc explain output?
There was a problem hiding this comment.
Good question, we generally don't have cross-field examples, but I guess in this case, you cannot specify arbitrary curves that the ciphers you have don't support?
Would anything validate that the curves list is correct for the ciphers you have? Or are all curves supported by all ciphers? Would there ever be a need to say, specify a subset of curves per cipher?
(This might be completely invalid, but as an example, you would want X25519MLKEM768 for TLS_CHACHA20_POLY1305_SHA256, and P-384 for TLS_AES_256_GCM_SHA384, and only those combinations)
There was a problem hiding this comment.
IMO the user facing docs call out that the custom TLS profile "can cause some problems" if misconfigured which is a good hint to the administrator that they should:
- Know what they are doing
- Validate their config before using the profile in a production environment
I think the best approach here is to offer reasonable valid defaults while keeping custom profiles as a "power user" option.
There was a problem hiding this comment.
Maybe the example is better suited to the higher-order custom field since it is showing a more holistic configuration example?
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=5 | ||
| Curves []TLSCurve `json:"curves,omitempty"` |
There was a problem hiding this comment.
Does it make sense to allow duplicates, or should we have some validation to prevent that (something like // +kubebuilder:validation:XValidation:rule=`self.all(x, self.exists_one(y, x == y))`,message="curves cannot contain duplicates")?
There was a problem hiding this comment.
I think it is a really good idea!
What do you think if we switch to listType=set
There was a problem hiding this comment.
I think sets should work to ensure that only one entry of each TLSCurve exists
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
eba1e60 to
4c8e011
Compare
@Miciah |
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
8230b44 to
963c42d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
303-344:⚠️ Potential issue | 🟠 Major
tlsSecurityProfile.custommentions curves but schema lackscurves.
The description states curve behavior, but the schema only exposesciphersandminTLSVersion, so users cannot configure curves as documented. Either add thecurvesfield to the schema (preferred for this feature set) or remove the curve references.Suggested schema addition
properties: ciphers: description: |- ciphers is used to specify the cipher algorithms that are negotiated during the TLS handshake. Operators may remove entries their operands do not support. For example, to use DES-CBC3-SHA (yaml): @@ items: type: string type: array x-kubernetes-list-type: atomic + curves: + description: |- + curves is used to specify supported elliptic curves negotiated during + the TLS handshake. + items: + type: string + enum: + - X25519 + - P-256 + - P-384 + - P-521 + - X25519MLKEM768 + - P256r1MLKEM768 + - P384r1MLKEM1024 + maxItems: 7 + type: array + x-kubernetes-list-type: set minTLSVersion:payload-manifests/crds/0000_10_config-operator_01_apiservers-OKD.crd.yaml (1)
233-276:⚠️ Potential issue | 🟠 Major
tlsSecurityProfile.custommentions curves but schema lackscurves.
The description claims curve behavior, but the schema only exposesciphersandminTLSVersion, so users cannot set curves as described. Add thecurvesfield (or drop the curve references) to keep the CRD contract consistent.Suggested schema addition
properties: ciphers: description: |- ciphers is used to specify the cipher algorithms that are negotiated during the TLS handshake. Operators may remove entries their operands do not support. For example, to use DES-CBC3-SHA (yaml): @@ items: type: string type: array x-kubernetes-list-type: atomic + curves: + description: |- + curves is used to specify supported elliptic curves negotiated during + the TLS handshake. + items: + type: string + enum: + - X25519 + - P-256 + - P-384 + - P-521 + - X25519MLKEM768 + - P256r1MLKEM768 + - P384r1MLKEM1024 + maxItems: 7 + type: array + x-kubernetes-list-type: set minTLSVersion:
🤖 Fix all issues with AI agents
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml`:
- Around line 234-240: Update the description for the "custom" TLS security
profile in the Default feature set to clarify that the CRD variant does not
expose a "curves" field and therefore does not set an explicit curve list; state
that curve preferences are not configurable here and that the underlying
component/default implementation will apply its own curve defaults instead of
implying an empty/deny-all list for "custom". Reference the "custom" profile
description block so the new wording explicitly replaces "The curve list for
this profile is empty by default." with a sentence that makes the above behavior
clear.
🧹 Nitpick comments (1)
config/v1/types_tlssecurityprofile.go (1)
112-116: Clarify default behavior for omitted curves.To avoid a “deny‑all” read, mirror the Curves field docs: omitted means no opinion and platform defaults apply.
✏️ Suggested wording
- // The curve list for this profile is empty by default. + // If the curves list is omitted, the platform applies component defaults (no opinion).
| description: |- | ||
| custom is a user-defined TLS security profile. Be extremely careful using a custom | ||
| profile as invalid configurations can be catastrophic. An example custom profile | ||
| looks like this: | ||
| profile as invalid configurations can be catastrophic. | ||
|
|
||
| The curve list for this profile is empty by default. | ||
|
|
||
| An example custom profile looks like this: |
There was a problem hiding this comment.
Clarify curve defaults for Custom profile in the Default feature set.
This CRD variant doesn’t expose curves, so “empty by default” can be read as deny‑all. Consider stating that curve preferences aren’t configurable here and that component defaults apply.
✏️ Suggested wording
- The curve list for this profile is empty by default.
+ Curve preferences are not configurable in this feature set; when omitted the platform applies component defaults.🤖 Prompt for AI Agents
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-Default.crd.yaml`
around lines 234 - 240, Update the description for the "custom" TLS security
profile in the Default feature set to clarify that the CRD variant does not
expose a "curves" field and therefore does not set an explicit curve list; state
that curve preferences are not configurable here and that the underlying
component/default implementation will apply its own curve defaults instead of
implying an empty/deny-all list for "custom". Reference the "custom" profile
description block so the new wording explicitly replaces "The curve list for
this profile is empty by default." with a sentence that makes the above behavior
clear.
| // - P-256 | ||
| // | ||
| // +optional | ||
| // +listType=set |
There was a problem hiding this comment.
Let's document that entries to this list must be unique as part of the field documentation.
davidesalerno
force-pushed
the
addCurvesForECDH
branch
2 times, most recently
from
a986104 to
afd6553
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
payload-manifests/crds/0000_80_machine-config_01_kubeletconfigs-OKD.crd.yaml (1)
117-145:⚠️ Potential issue | 🟡 MinorDescription mentions curves but schema doesn't expose
curvesfield.The OKD CRD's
tlsSecurityProfile.customdescription references "curve list" behavior, but this schema doesn't include acurvesproperty (onlyciphersandminTLSVersion). SinceTLSCurvePreferencesis not enabled for OKD, this is expected, but the description text is misleading.Consider removing the curve-related description from the Custom profile section, or clarifying that curve configuration is not available in this feature set.
Suggested wording for Custom profile
custom: description: |- custom is a user-defined TLS security profile. Be extremely careful using a custom - profile as invalid configurations can be catastrophic. - - The curve list for this profile is empty by default. - - An example custom profile looks like this: + profile as invalid configurations can be catastrophic. An example custom profile looks like this:
🤖 Fix all issues with AI agents
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/TLSCurvePreferences.yaml`:
- Around line 205-225: The test is brittle because the curves list uses set
semantics so the duplicate "X25519" may be deduplicated before maxItems is
checked; update the test data in the IngressController fixture so the curves
list contains six distinct entries (or remove the duplicate X25519) to
deterministically trigger the max-items validation on
spec.tlsSecurityProfile.custom.curves, or alternatively change the schema/test
to use array semantics; ensure the fixture under IngressController uses unique
curve values when asserting the "Too many" error.
| initial: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: IngressController | ||
| metadata: | ||
| name: default | ||
| namespace: openshift-ingress-operator | ||
| spec: | ||
| tlsSecurityProfile: | ||
| type: Custom | ||
| custom: | ||
| minTLSVersion: VersionTLS12 | ||
| ciphers: | ||
| - TLS_AES_128_GCM_SHA256 | ||
| curves: | ||
| - X25519 | ||
| - SecP256r1 | ||
| - SecP384r1 | ||
| - SecP521r1 | ||
| - X25519MLKEM768 | ||
| - X25519 | ||
| expectedError: "spec.tlsSecurityProfile.custom.curves: Too many: 6: must have at most 5 items" |
There was a problem hiding this comment.
Max-items test is brittle with listType=set.
With set semantics and only five allowed curve values, the duplicate X25519 can trigger uniqueness validation before maxItems, so the expected “Too many” error can be flaky. Consider removing this case or aligning schema/test so maxItems can be validated deterministically.
🤖 Prompt for AI Agents
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/TLSCurvePreferences.yaml`
around lines 205 - 225, The test is brittle because the curves list uses set
semantics so the duplicate "X25519" may be deduplicated before maxItems is
checked; update the test data in the IngressController fixture so the curves
list contains six distinct entries (or remove the duplicate X25519) to
deterministically trigger the max-items validation on
spec.tlsSecurityProfile.custom.curves, or alternatively change the schema/test
to use array semantics; ensure the fixture under IngressController uses unique
curve values when asserting the "Too many" error.
| minTLSVersion: VersionTLS12 | ||
| ciphers: | ||
| - TLS_AES_128_GCM_SHA256 | ||
| curves: [] |
There was a problem hiding this comment.
If you just remove the field, is this allowed?
There was a problem hiding this comment.
It looks like it is. Lets add a test that removing the field altogether is allowed
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Aside from a couple minor comments, this LGTM.
openshift-merge-robot
added
the
needs-rebase
…rves in TLSProfile Signed-off-by: Davide Salerno <dsalerno@redhat.com>
Co-authored-by: Bryce Palmer <everettraven@gmail.com>
Signed-off-by: Davide Salerno <dsalerno@redhat.com>
Signed-off-by: Davide Salerno <dsalerno@redhat.com>
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
d367552 to
b2bbee4
Compare
openshift-merge-robot
removed
the
needs-rebase
Regenerate CRD manifests, OpenAPI definitions, and swagger docs after rebasing on upstream/masr Signed-off-by: Davide Salerno <dsalerno@redhat.com>
davidesalerno
force-pushed
the
addCurvesForECDH
branch
from
b2bbee4 to
b678623
Compare
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
The failure is an existing issue that wouldn't normally block a PR. Overriding due to existing issue. /override ci/prow/verify-crd-schema |
|
/test build |
|
PR-Agent: could not fine a component named |
|
@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-crd-schema DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@davidesalerno: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| const ( | ||
| // TLSCurveX25519 represents X25519. | ||
| TLSCurveX25519 TLSCurve = "X25519" | ||
| // TLSCurveSecp256r1 represents P-256 (secp256r1). | ||
| TLSCurveSecP256r1 TLSCurve = "SecP256r1" | ||
| // TLSCurveSecP384r1 represents P-384 (secp384r1). | ||
| TLSCurveSecP384r1 TLSCurve = "SecP384r1" | ||
| // TLSCurveSecP521r1 represents P-521 (secp521r1). | ||
| TLSCurveSecP521r1 TLSCurve = "SecP521r1" | ||
| // TLSCurveX25519MLKEM768 represents X25519MLKEM768. | ||
| TLSCurveX25519MLKEM768 TLSCurve = "X25519MLKEM768" | ||
| ) |
There was a problem hiding this comment.
I'm no crypto expert to take my questions and assumptions with a grain of salt here.
Is there a specific reason why are going with IANA names for Curves here whereas for Ciphers we went with OpenSSL names?
My understanding is that Go uses IANA as a reference for both Ciphers and Curves: https://github.com/golang/go/blob/96e4e2b1616c3c59577d48abcf2823bf1fdcd2e2/src/crypto/tls/common.go#L139-L154
I was under the impression that for Ciphers we originally went with OpenSSL names (rather than IANA) so that non go network components like HAProxy, kube-rbac-proxy, ... & other components can use the OpenSSL names natively, whereas for go we can always put them through the library-go's crypto.OpenSSLToIANACipherSuites() to get the IANA formatted ciphers names to use with go's tls.Config.
For these Curves instead it looks to me we are going the other way around?
Wouldn't it be more consistent to go with OpenSSL names by default in the API and then have a crypto.OpenSSLToIANACurvePreferences() to get the IANA formatted curve names?
@davidesalerno @everettraven @joelanford @kannon92 @sanchezl ?
Thanks
There was a problem hiding this comment.
I could be missing something, but I couldn't find a canonical list of OpenSSL curve names where there is a clear deviation from the IANA names.
I don't have any strong opinions here, but if there is a clear difference I think sticking with the ciphers convention makes sense here.
There was a problem hiding this comment.
Curves are user-visible fields that we'd like to keep in the most common user-visible formatting, and IANA is the best source for that. We don't need to keep consistent with cipher formatting if the format that was used there was not what we would pick today.
@davidesalerno please change the string constant using the same case as in https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8, it doesn't make sense to use camel case on user-input strings. We may need to make sure that validation ignores case as well, if it doesn't already.
If this was the last item on the review list, can we please proceed asap with this PR?
There was a problem hiding this comment.
We have had a longer internal conversation on this here: https://redhat-internal.slack.com/archives/C098FU5MRAB/p1770317849553639
Let's coordinate there @candita @davidesalerno @everettraven
15 participants
This change will add a new Curves field to the TLSProfile specification.
This is required in order to support new PQC curves, we need a config for explicitly setting the supported elliptic curves algorithms ("curve suite") that are negotiated during the SSL/TLS handshake with ECDHE.
This PR is related to openshift/cluster-ingress-operator#1287 and openshift/router#678